criu: replace deprecated strings.Title #4914
Conversation
libcontainer/criu_linux.go
Outdated
| nsName := configs.NsName(t) | ||
| // Capitalize first letter manually to avoid deprecated strings.Title | ||
| if len(nsName) > 0 { | ||
| nsName = strings.ToUpper(nsName[:1]) + nsName[1:] | ||
| } | ||
| return "extRoot" + nsName + "NS" |
There was a problem hiding this comment.
This looks kind of ugly, but I came up with this:
func criuNsToKey(t configs.NamespaceType) string {
// Construct "extRoot" + capitalize(nsName) + "NS" without allocations.
// Result format: "extRootNetNS", "extRootPidNS", etc.
nsName := configs.NsName(t)
out := make([]byte, 0, 64)
out = append(out, "extRoot"...)
// Capitalize the first character (this assumes it's in the a-z range).
out = append(out, nsName[0]-'a'+'A')
out = append(out, nsName[1:]...)
out = append(out, "NS"...)
return string(out)
}TBH I wish we can just drop the capitalization, but we'll lose backward compatibility (restore from old version won't work).
There was a problem hiding this comment.
won't it panic if nsName is empty; accessing "nsName[0]", in out = append(out, nsName[0]-'a'+'A')?
There was a problem hiding this comment.
Yes it will. This is a simplified code which assumes that nsName is not empty and its first character is a lowercase ASCII. I think these assumptions are correct given the current codebase, but of course it's better be safe than sorry. So please add a check.
I also wrote a benchmark to see how fast is the code and how many allocations it does, and the above is the fastest I could do.
There was a problem hiding this comment.
While manually fiddling with bytes is probably more efficient, I would've preferred using strings.Builder to be honest.
Also, nsName[0]-'a'+'A' is kinda ugly -- I would use unicode.ToUpper (which does that internally for ASCII characters).
There was a problem hiding this comment.
Using strings.Builder for a 16 character string is an overkill.
Using unicode.ToUpper is fine I guess.
// Capitalize the first character.
out = append(out, byte(unicode.ToUpper(rune(nsName[0]))))
osamakader
force-pushed
the
fix-stringsTitle-criu-linux
branch
from
4a852dd to
3a0b5e8
Compare
osamakader
force-pushed
the
fix-stringsTitle-criu-linux
branch
from
3a0b5e8 to
db302e2
Compare
My pleasure :) |
osamakader
force-pushed
the
fix-stringsTitle-criu-linux
branch
from
db302e2 to
418e4e3
Compare
|
@osamakader Please update your commit message subject to: "criu: replace deprecated strings.Title", as it should be kept under 72 characters. Thanks. @kolyshkin @osamakader Just to discuss—why not use methods from Go's standard packages or official libraries? For example:
While this manual capitalization approach is simple, I strongly recommend using standard library methods. That said, it's not a blocker for merging. |
I'd rather not introduce another dependency for such trivial stuff. |
That makes sense. What do you think about this approach: |
|
@osamakader thanks! I agree with @lifubang and @cyphar here. Let's try to find a simpler way to do this. x/text is even suggested in the documentation for the deprecated function. Can you explore the options suggested by others and see what is simpler for this use case? |
osamakader
force-pushed
the
fix-stringsTitle-criu-linux
branch
from
418e4e3 to
f4c7571
Compare
|
strings.Title is deprecated since Go 1.18. Replace it with a simple manual capitalization of the first character in criuNsToKey(). Signed-off-by: Osama Abdelkader <[email protected]> Co-authored-by: Kir Kolyshkin <[email protected]>
osamakader
force-pushed
the
fix-stringsTitle-criu-linux
branch
from
f4c7571 to
1adb070
Compare
osamakader
changed the title
5 participants
Replaces the deprecated strings.Title call in criuNsToKey() with a manual capitalization of the first character.
The behavior remains identical for ASCII namespace names while ensuring compatibility with modern Go versions.